Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tab index to close icon #48

Merged
merged 8 commits into from
Apr 11, 2018
Merged

Conversation

peterlazzarino
Copy link
Contributor

PR for issue #36 @pradel I'd like your thoughts on this. React currently has a known issue in production that is resolved in a beta branch that prevents SVG elements from properly rendering the tabindex attribute without camel case which prevents it from working currently.

facebook/react#10987

Tab index here is set to 0 so it doesn't interfere with any tabindex attributes set on modal content such as forms, etc. The only potential issue I can see is that since the close button is first in the element list, it will be the first tab option before anything in the modal. This could be resolved by moving the close button below the body content since it is absolute positioned.

@peterlazzarino peterlazzarino changed the title Add tab index to SVG Add tab index to close icon Feb 22, 2018
@pradel
Copy link
Owner

pradel commented Feb 23, 2018

Hum I am not an expert in tabIndex so I can't think about other side effect
any opinion @artemjackson?

Could you move the close icon please? I will try after that :)

@peterlazzarino
Copy link
Contributor Author

@pradel all set, just moved it and it's working great for me. I'm able to tab to X after tabbing through form content.

@artemjackson
Copy link

@pradel @peterlazzarino
I would recommend to wrap SVG with an button, cuz button catches "Tab"-ing by default and emits click event on space or enter hit.

@pradel
Copy link
Owner

pradel commented Feb 28, 2018

@artemjackson we should definitely wrap it with a button will be easier you are right. @peterlazzarino does that looks good to you?

@peterlazzarino
Copy link
Contributor Author

@pradel I was playing around with this yesterday however that button would have to be the one that is styled absolute, top, right, etc instead of the SVG. The closeIcon class would no longer behave as it does now resulting in some potentially breaking changes for consumers. This is also complicated by the SVG size prop that is passed in to the component.

I was thinking a good solution to keep as much backwards compatibility as possible would be to wrap the SVG with a button that has a tabIndex and a new closeButton class style that has the absolute positioning. The user could override the closeIcon and style it like they can now and also still pass the height and width for the SVG but to move it around they'd have to style the button.

What do you think?

@pradel
Copy link
Owner

pradel commented Mar 1, 2018

@peterlazzarino would be great to keep backward compatibility if possible
Your solution seems like a good one :)

@pradel
Copy link
Owner

pradel commented Apr 11, 2018

@peterlazzarino does that looks good to you?
I will also fix other bugs and release as a major version 3.0.0 since this can be a breaking change for some users

@pradel pradel merged commit c02b562 into pradel:master Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants